src/CompositeKey.SourceGeneration/Parser.cs (3)

28-36: ReportDiagnostic is public on an internal class — consider internal visibility.

Since Parser is internal sealed, public methods on it are effectively internal. However, marking it internal (or leaving it at the default for an internal class context) would be more intentional about the intended access scope.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/CompositeKey.SourceGeneration/Parser.cs` around lines 28 - 36, The
ReportDiagnostic method on the internal sealed Parser class is declared public;
change its visibility to internal to match the class scope and make intent
explicit. Update the ReportDiagnostic method declaration (method name:
ReportDiagnostic in Parser.cs) from public to internal, keeping the existing
parameters and body unchanged, so the method is only visible within the assembly
and consistent with the internal sealed Parser class.

---

577-624: ParseCompositeKeyAttributeValues iterates all attributes but does not break after finding a match.

The Debug.Assert at line 585 will fire in debug builds if there are somehow two CompositeKeyAttribute instances, but in release builds the loop will silently overwrite attributeValues. Since the attribute should have AllowMultiple = false, this is unlikely, but an early break after the match would make the intent clearer.

♻️ Suggested change

             if (SymbolEqualityComparer.Default.Equals(attributeData.AttributeClass, _knownTypeSymbols.CompositeKeyAttributeType))
+            {
                 attributeValues = TryExtractAttributeValues(attributeData);
+                break;
+            }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/CompositeKey.SourceGeneration/Parser.cs` around lines 577 - 624, The loop
in ParseCompositeKeyAttributeValues may continue after finding a matching
CompositeKeyAttribute and overwrite attributeValues in release builds; after
assigning attributeValues = TryExtractAttributeValues(attributeData) inside the
foreach where
SymbolEqualityComparer.Default.Equals(attributeData.AttributeClass,
_knownTypeSymbols.CompositeKeyAttributeType) is true, add an immediate break to
exit the loop (preserving the existing Debug.Assert) so the first-found
attribute value is retained and no subsequent attributes can overwrite it.

---

15-15: Consider making CompositeKeyAttributeValues internal.

This record is only consumed by Parser (internal) and is within an analyzer assembly (shipped with PrivateAssets=all), so public visibility is broader than necessary. Changing to internal would reduce the public API surface of the analyzer assembly.

♻️ Suggested change

-public sealed record CompositeKeyAttributeValues(string TemplateString, char? PrimaryKeySeparator, bool InvariantCulture);
+internal sealed record CompositeKeyAttributeValues(string TemplateString, char? PrimaryKeySeparator, bool InvariantCulture);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/CompositeKey.SourceGeneration/Parser.cs` at line 15, The
CompositeKeyAttributeValues record is unnecessarily public; change its
declaration from public to internal so it becomes internal sealed record
CompositeKeyAttributeValues(string TemplateString, char? PrimaryKeySeparator,
bool InvariantCulture); and ensure any usages (e.g., in the internal Parser
type) still compile—adjust accessibility only on the CompositeKeyAttributeValues
declaration to reduce the analyzer assembly's public API surface.

src/CompositeKey.SourceGeneration/Emitter.cs (1)

374-381: Hardcoded stackalloc Range[128] limits repeating property collections to ~128 items.

When a collection has more than 128 items and uses the same separator as key delimiters, ReadOnlySpan<char>.Split will pack the remainder into the last range. This leads to a confusing FormatException rather than a clear error. While 128 is generous for typical usage, this is worth being aware of.

If this is an intentional design constraint, consider adding a comment documenting the limit. If it should be unbounded, consider an ArrayPool<Range>-based approach.

Also applies to: 560-568

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/CompositeKey.SourceGeneration/Emitter.cs` around lines 374 - 381, The
stackalloc fixed buffer (stackalloc Range[128]) can overflow for large repeating
collections; replace it with a rented array from ArrayPool<Range> (e.g., var
rented = ArrayPool<Range>.Shared.Rent(capacity); try { Span<Range>
{partRangesVariableName} = rented; int {partCountVariableName} =
{inputName}.{method}(...); ... } finally {
ArrayPool<Range>.Shared.Return(rented); }) so the split can handle >128 parts;
apply the same change for the other occurrence (lines ~560-568) and ensure the
rented array is returned in a finally block and the Span is created from the
rented array before calling the split.

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro